Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update audit error message for versioned formulae with head spec #7980

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

dtrodrigues
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Based on the existing code and the discussion at #5827, it appears the intention is to only disallow HEAD in versioned formulae, not all formulae. This PR updates the error message accordingly.

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I do have a question about this audit (which is a little out of scope for this PR but the old one is closed so I think it makes sense to post it here):

@MikeMcQuaid said:

The only situation in which head makes sense in versioned formulae is if it points to a versioned e.g. release branch that will be used to make the next patch version in that release series. I'd agree that if we allow it a whitelist is the only sensible way.

According to the docs, however, for all versioned formulae:

Upstream should have a release branch for each formula version, and release security updates for each version when necessary.

Doesn't this mean that all versioned formulae, which must have release branched for each version, should be able to have a head block?

There's an allowlist anyway, so this probably doesn't matter much and I don't want to reopen a discussion if it's already been decided.

Copy link
Contributor

@jonchang jonchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this fine, we can add it to the permitted list for the few versioned formulae that quality. Thanks!

@jonchang jonchang merged commit bc4a108 into Homebrew:master Jul 12, 2020
@MikeMcQuaid
Copy link
Member

Doesn't this mean that all versioned formulae, which must have release branched for each version, should be able to have a head block?

Technically: yes. I'm happy to see the documents updated accordingly. A previous maintainer had strong views on not having head in versioned formulae but I do not.

@Rylan12
Copy link
Member

Rylan12 commented Jul 20, 2020

I don't have any strong opinions either, I was just curious. I see no reason to re-evaluate at this point. I think if someone wants it enough, they can bring it up and we can think about it again then.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 23, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants